Fixes the DedupLink shared observable unsubscribing early when shared #984
Conversation
|
@ms: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/ |
The DedupLink creates a real (single) observable to listen to when a request is made and returns a shared observable to its caller. When a second identical request is made, the real observable is left untouched but the subscriber's callback functions is added to the list of callbacks to be triggered when the request resolves and the shared observable is returned. Unfortunately the shared observable returns cleanup logic that unsubscribes from the real observable without checking whether only one of many (shared) subscribers is unsubscribing. In the case where DedupLink is in front of HttpLink, this leads to HttpLink aborting the HTTP request while some callers are still waiting for a response. This change modifies the sharing code to use a Set of observers and only unsubscribe from the underlying Observable when the last observer is removed.
Codecov Report
@@ Coverage Diff @@
## master #984 +/- ##
==========================================
- Coverage 95.2% 95.19% -0.01%
==========================================
Files 22 22
Lines 1022 1020 -2
Branches 104 92 -12
==========================================
- Hits 973 971 -2
Misses 44 44
Partials 5 5
Continue to review full report at Codecov.
|
|
Just to add a bit more context around this: we were running into issues where requests were being made successfully in The result was that every query observable ended up with two subscribers: the one created in the A related question I have is whether it makes sense to use a |
Since PR #4576 removes the dependency on apollo-link-dedup by replacing it with an internal implementation, this recent apollo-link-dedup PR caught my attention: apollographql/apollo-link#984 After adapting the regression test from that PR, it became clear that multiplexing should be the default behavior for all link Observables. In other words, the underlying observable.subscribe method should be called at most once, yet the results/errors should be delivered to all observers without actually subscribing each observer to the underlying observable. The single shared subscription gets unsubscribed once all observers have unsubscribed. When I tried to adopt this policy previously (since it is required for subscriptions, and seemed like a good idea for queries and mutations too), a single test failed ('returns the same value as observableQuery.next got'), but with some additional effort this time I was able to track that down to observable.refetch() accidentally reusing an old (deduplicated) observable because the new deduplication logic wasn't calling the cleanup function after the first next event on the underlying observable. Once I fixed that, all tests passed.
Context
The DedupLink creates a real (single) observable to listen to when a
request is made and returns a shared observable to its caller. When a
second identical request is made, the real observable is left untouched
but the subscriber's callback functions is added to the list of
callbacks to be triggered when the request resolves and the shared
observable is returned.
Problem
Unfortunately the shared observable returns cleanup logic that
unsubscribes from the real observable without checking whether only one
of many (shared) subscribers is unsubscribing. In the case where
DedupLink is in front of HttpLink, this leads to HttpLink aborting the
HTTP request while some callers are still waiting for a response.
Fix
This change modifies the sharing code to use a Set of observers and only
unsubscribe from the underlying Observable when the last observer is
removed.
TODO: